Skip to content

Conversation

@Sigma1912
Copy link
Contributor

  • Restores the tooltips for [Delete, Add, Reload, Save] buttons
  • Restores blocking the user from deleting the loaded tool
  • Makes tooledit widget return a list rather than 'None' when multiple checkboxes are selected (required to check if loaded tool is among multiple selected tools)
  • Hides current tool number display in the tooledit frame when the touch keyboard is not active

@zz912
Copy link
Contributor

zz912 commented Jan 18, 2026

Should be possible add to this PR functionality from #3474 ?

@Sigma1912
Copy link
Contributor Author

Should be possible add to this PR functionality from #3474 ?

I'll have a look

Comment on lines 2043 to 2052
lbl_tool_text = "Tool loaded: " + str(toolnumber)
lbl_tool_text = ""
# When using touch keyboard we display the current tool in the tooltable frame
if self.widgets.chk_use_kb_on_tooledit.get_active():
lbl_tool_text = "Tool loaded: " + str(toolnumber)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not always display the tool number of the current tool?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a request to avoid having redundant information:
#3509 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok. I disagree 🙃

@hansu
Copy link
Member

hansu commented Jan 18, 2026

  1. Removing the calls of self.widgets.tooledit1.set_selected_tool(act_tool) improves the behaviour in most cases but for on_reload_clicked() I would like to keep it. So that the tool in the spindle is checked when opening the tool table and on reload.
    What do you think?

  2. The "reload tooltable when the widget is shown" has one disadvantage: when you add a new tool, and go back without saving it, your changes got lost. What is the benefit from reload when showing?

@Sigma1912
Copy link
Contributor Author

I would like to keep it. So that the tool in the spindle is checked when opening the tool table and on reload.
What do you think?

No problem

What is the benefit from reload when showing?

Current behavior is that unsaved changes in the tool table are retained when leaving the tool table screen but the changed values are discarded when a tool change is executed and the saved offsets are used instead.
IMO this is unexpected behavior. When I open the tool table I expect to see the values that will be applied when a tool is changed.
Maybe the user should be presented with a popup message if there are unsaved changes on leaving the page?

@zz912
Copy link
Contributor

zz912 commented Jan 18, 2026

I think the reolading is good idea. But I did not test it yet.

@hansu
Copy link
Member

hansu commented Jan 18, 2026

I would like to keep it. So that the tool in the spindle is checked when opening the tool table and on reload.
What do you think?

No problem

👍

Maybe the user should be presented with a popup message if there are unsaved changes on leaving the page?

That would be the cleanest solution.
Then we might also save the tool table when doing a tool change. Imagine someone changes the offsets of a tool and then initiates a tool change for that tool. Then the newly entered offsets wouldn't be used.
Or add another warning message like "Tool table contains unsaved changes. They will get lost after the Tool change."

@Sigma1912 Sigma1912 force-pushed the Gmoccapy-tooltable_Restore_button_tooltips branch from 2d5a5b9 to eff3d22 Compare January 19, 2026 14:03
@Sigma1912
Copy link
Contributor Author

  • Dropped the commit hiding the label for current tool number in the tool table frame
  • Added message on leaving the page with unsaved changes (either by 'back' button or by change tool buttons)

@Sigma1912
Copy link
Contributor Author

Sigma1912 commented Jan 19, 2026

Then we might also save the tool table when doing a tool change.

I opted for the message popup as I prefer the user to be aware of what is going on.

@Sigma1912 Sigma1912 force-pushed the Gmoccapy-tooltable_Restore_button_tooltips branch from eff3d22 to ffcaa40 Compare January 19, 2026 15:47
@hansu
Copy link
Member

hansu commented Jan 19, 2026

It seems something went wrong. The first two commits doesn't belong in this PR and there is also a merge conflict...

@zz912
Copy link
Contributor

zz912 commented Jan 19, 2026

I tested popup. It works.
Is it possible to respond with a popup to a cell that has been filled out when trying to leave?
tool_table

@Sigma1912 Sigma1912 force-pushed the Gmoccapy-tooltable_Restore_button_tooltips branch from ffcaa40 to d27a24e Compare January 19, 2026 18:10
@hansu
Copy link
Member

hansu commented Jan 19, 2026

And to be a bit pedantic - I would change the message "Discard changes?" (when changing a tool with unsaved changes) to "Change tool without saving changes/ and discard changes?"
Otherwise it could be read as "save changes and change tool". What do you think?

And the warning pop up could also be added for reload.

@Sigma1912
Copy link
Contributor Author

Sigma1912 commented Jan 19, 2026

Is it possible to respond with a popup to a cell that has been filled out when trying to leave?

I have not found a solution for that yet.

@Sigma1912 Sigma1912 force-pushed the Gmoccapy-tooltable_Restore_button_tooltips branch from d27a24e to e217db4 Compare January 19, 2026 18:39
@hansu
Copy link
Member

hansu commented Jan 19, 2026

Looks good now, thanks. Only the buildbot looks weird.

@hansu
Copy link
Member

hansu commented Jan 19, 2026

Let me rebase that to master and see if that makes the buildbot/CI happy...

@hansu hansu force-pushed the Gmoccapy-tooltable_Restore_button_tooltips branch from e217db4 to d42a12d Compare January 19, 2026 21:29
@BsAtHome
Copy link
Contributor

Let me rebase that to master and see if that makes the buildbot/CI happy...

CI fails on Debian:Sid telling us that there no longer is a working c++ compiler:

...
checking whether the C++ compiler works... no
configure: error: in '/__w/linuxcnc/linuxcnc/src':
configure: error: C++ compiler cannot create executables
See 'config.log' for more details
...

The rest of CI seems to work fine. Probably someone pushed an update to the CI infrastructure that broke stuff.

@Sigma1912
Copy link
Contributor Author

Fixes the bug with the Diameter value not updating when changing the active tool diameter in the table:

Recording

@zz912
Copy link
Contributor

zz912 commented Jan 20, 2026

Thank you.

Is offset updated too?

@Sigma1912
Copy link
Contributor Author

Is offset updated too?

Not yet. I want to fix the obvious bugs first.
I'm still pondering the 'G43' issue as I don't like it when the GUI silently issues MDI commands when I click on a button like that.

@Sigma1912
Copy link
Contributor Author

Is offset updated too?

Oh, did you mean the 'offset z' value?
That is NOT updated as this value is taken from 'motion.tooloffset.z' and reflects the actually applied offset. So this is '0.0' with 'G49'.
One might argue that it should just reflect the tool table value but personally I find it useful to have it like this.

@zz912
Copy link
Contributor

zz912 commented Jan 20, 2026

I think I've solved your questions in #3474.

  • If G49 is active, nothing happens.
  • This PR fixes the issue with mode switching when executing mdi.command. I think this solution is the best the current API offers.

I don't like it when the GUI silently issues MDI commands

First I wanted to write to you to make a popup question whether G43 should be executed when a value in the table is changed. But then I thought about it and I couldn't think of a situation where someone would change the offsets in the tooltable and still want to work with the old offsets.

It would be stupid if the "save button" activated G43 when G49 was active, but my PR doesn't do that.

Furthermore, during testing I found out that it should not be possible to change the tool diameter when G41 or G42 is on. The change should only be possible when G40 is on.
https://linuxcnc.org/docs/devel/html/gcode/g-code.html#gcode:g41-g42
https://linuxcnc.org/docs/devel/html/gcode/tool-compensation.html#sec:cutter-radius-compensation
g40_g41_g42

@zz912
Copy link
Contributor

zz912 commented Jan 21, 2026

The Popup looks good.
g43

I'm taking this from the perspective of a beginner learning to work with a CNC machine. I was in this situation recently. I change the offset, and nothing happens. Then I try to "save" and nothing happens again. As a beginner, it doesn't occur to me to "exit" to update the offsets.

Or I would like to have some other option to trigger the reactivation of G43 in the tooltable environment.

@hansu
Copy link
Member

hansu commented Jan 21, 2026

@hansu @andypugh Do you think reactivating G43 after changing offsets in the tooltable is a good idea?

The solution with the confirmation dialog is good. But it doesn't appear after the second change or if you apply G43 manually:

tooltable.mp4

@zz912
Copy link
Contributor

zz912 commented Jan 21, 2026

To hans:

Note that if you have G43 activated and you change the offset to 0, then the next time you reactivate G43, G49 will be activated. This feature of LCNC confused me a lot when I was learning to work with offsets.
It might be worth making another statement for the situation where the current tool offset is 0 that g49 will be activated.

@hansu
Copy link
Member

hansu commented Jan 21, 2026

Ah ok I didn't know/noticed that. My machine doesn't allow resp. it doesn't make sense to work with tool offsets.
So it's fine, sorry for the noise David.

@hansu
Copy link
Member

hansu commented Jan 21, 2026

Removing the calls of self.widgets.tooledit1.set_selected_tool(act_tool) improves the behaviour in most cases but for on_reload_clicked() I would like to keep it. So that the tool in the spindle is checked when opening the tool table and on reload.
What do you think?

No problem

As the selection row is only used for deleting, maybe it would be better to highlight the current tool in another way like having the line bold. But not sure if that is possible.

@Sigma1912
Copy link
Contributor Author

maybe it would be better to highlight the current tool in another way like having the line bold. But not sure if that is possible.

The idea was for the user to select the tool in the same way we select the offset in the offset page.
The issue with marking the currently loaded tool in the table is that it is not always visible as we scroll up/down. That is why we decided to have the label in the button box.

@Sigma1912
Copy link
Contributor Author

Or I would like to have some other option to trigger the reactivation of G43 in the tooltable environment.

Generally I'm not a great fan of GUIs that try to provide convenience functions for everything. The automatic reactivation of G43 after a tool change was also such a function until we got rid of it.

IMO allowing the operator to change the tool table entry of the tool in the spindle with 'G43' active is a bit dubious. In the end all of these things could just as well be done by the operator in the MDI page with the added benefit of actually having to think whats happening.

@zz912
Copy link
Contributor

zz912 commented Jan 21, 2026

The automatic reactivation of G43 after a tool change was also such a function until we got rid of it.

We didn't get rid of this feature, we just moved it to remap. This was due to the race condition and instability of this feature when it was in the GUI. "reactivation of G43 after a tool change" caused problems mainly with the combination of halui.mdicommands and multiple commands in mdi-commands (for example M6 T4 G4 P1). Reactivation of G43 in the tooltable is a different situation, I don't see any problem here.

Generally I'm not a great fan of GUIs that try to provide convenience functions for everything.

IMO allowing the operator to change the tool table entry of the tool in the spindle with 'G43' active is a bit dubious.

I had similar thoughts. After the first crash of my friend's machine, I reevaluated everything.

I don't know if you have a machine with an automatic tool changer and whether you use tool compensation.

I'm sorry that I keep adding more and more requests, but on the other hand it makes sense. You're making Gmoccapy and tooltable better and better. I'm convinced that it makes sense. Please be patient with me.

@Sigma1912
Copy link
Contributor Author

@hansu

In this selection scenario it's a bit unclear to which tool should be changed.

How about this:
Recording

@zz912
Copy link
Contributor

zz912 commented Jan 21, 2026

@hansu

In this selection scenario it's a bit unclear to which tool should be changed.

How about this: Recording

Should be possible change image "button delete"?
The first button image would look like a checkbutton.
The second button image would remain the same cross.

Tooltip should be changed too. First tooltip "Select for delete", second tooltip "delete"

@Sigma1912
Copy link
Contributor Author

Sigma1912 commented Jan 21, 2026

Changing the tooltips is no problem.

Changing icons is also possible but more work.

@hansu
Copy link
Member

hansu commented Jan 21, 2026

In this selection scenario it's a bit unclear to which tool should be changed.

How about this

This feels a bit weird. What I could imagine is to have the delete button as toggle button to enable delete mode and then to have small delete icons in every row which deletes the tool immediately:

grafik

But I am also fine with the current design with the checkbox. Just to get the column's title changed. Can't we change the title in the glade file, or is it only for Gmoccapy a delete-only checkbox?

@Sigma1912
Copy link
Contributor Author

Sigma1912 commented Jan 21, 2026

What I could imagine is to have the delete button as toggle button to enable delete mode and then to have small delete icons in every row which deletes the tool immediately

Yes, why not. Not sure how easy it is to modify the checkbox to an icon button.

Can't we change the title in the glade file, or is it only for Gmoccapy a delete-only checkbox?

Original behavior was for this column to select the rows to delete and the tool to change to. I have not found a way to change the column title other than in the 'toolwidget.glade' file.

@Sigma1912
Copy link
Contributor Author

According to this it is unfortunately not possible to replace the checkbox with a button:
https://discourse.gnome.org/t/cellrendererbutton-have-a-clickable-button-in-a-treeview-column/2103

@zz912
Copy link
Contributor

zz912 commented Jan 21, 2026

I have no problem with double funkcionality first button.
For select, it could look like:
check_button

For delete as is:
delete

Avoids confusion about using the 'Select' column to select tool to change to
@Sigma1912 Sigma1912 force-pushed the Gmoccapy-tooltable_Restore_button_tooltips branch from 746679c to 75da637 Compare January 22, 2026 15:28
@hansu
Copy link
Member

hansu commented Jan 22, 2026

I currently tend to drop the checkbox column at all and allow multi selection only with Ctrl+click or Shift+click.

We could also add a button to switch to multi selection mode that behaves like Ctrl is pressed. But I think it's a rare use case that you want to delete multiple tools at once IMHO.

@hansu hansu marked this pull request as draft January 25, 2026 17:47
hansu added 2 commits January 28, 2026 12:32
The selection appeared in the "wear-offsets" tab instead of the !all offsets" tab.
@hansu
Copy link
Member

hansu commented Jan 28, 2026

I would go for this:
tooltable-new-delete-selection-1

I don't think we need a multi-selection. It's still quite quick to delete multiple tools:
tooltable-new-delete-all

@hansu hansu marked this pull request as ready for review January 28, 2026 13:46
@zz912
Copy link
Contributor

zz912 commented Jan 28, 2026

I have no problems with single selection.

@hansu
Copy link
Member

hansu commented Jan 28, 2026

Great, let's merge it and see what the community says 🙃

@hansu hansu merged commit 250549e into LinuxCNC:master Jan 28, 2026
14 checks passed
@Sigma1912 Sigma1912 deleted the Gmoccapy-tooltable_Restore_button_tooltips branch January 29, 2026 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants